Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull lots of fixes and improvements from Springbuk's fork #1

Merged
merged 69 commits into from
Aug 22, 2023

Conversation

divoxx
Copy link
Member

@divoxx divoxx commented Aug 11, 2023

  • Tests are passing in Docker container
  • Reenable Rubocop, add encoding config parameter (Reenable Rubocop, add encoding config parameter localytics/odbc_adapter#29)
  • Handle connection failure by trying to reconnect and translating the error to ConnectionFailedError so the caller knows to retry
  • Bump version
  • Change gemspecs and gemfile
  • Upgrade to rails5.2.1
  • Upgrade to rails 6
  • Backward compatibility
  • Update bind params
  • Change params for Column class initializer
  • Support json type field
  • Support json and date datatype
  • Typo fix
  • Multi-database support
  • [APP-2051] Update Column#initialize to pass on **kwargs (rails 6.1 compatibility)
  • Update schema_statements.rb
  • Corrections for handling names
  • Oops
  • Changes because arg list to ActiveRecord::ConnectionAdapters::Column.new changed
  • Change to return primary key name as lowercase
  • Changes for columns function to return better types
  • [APP-2295] Update signature of translate_exception for ruby 3
  • Correct column default values
  • Add better return types
  • Clean up dev code
  • Add better handling of other types of null
  • Claims page was broken, discovered the bug is in the ODBC Adapter. dbms_type_cast needs to return rows at the end. Also added code to raise exceptions, since currently these circumstances shouldn't be encountered and if that changes spontaneously (such as through an snowflake odbc driver update) we want to know rather than silently have nil values.
  • Changes to eliminate garbage collection warning
  • MOJ-137 Can't quote error is caused by active_record not knowing what types columns are Updated infrastructure needed for models to know what types their attributes are so it will properly handle type casting automatically. This also removes the necessity of specifying json attributes on models. The only reason that dbms_type_cast is still useful is for when we're executing raw queries instead of using the object model.
  • MOJ-150 MOJ-151 Snowflake type mapping & array_of... types for Adds mapping for the remaining snowflake types, removes mapping for JSON type (as this isn't actually a type returned by snowflake ever) Add array_of_intgers, array_of_binaries, etc... types that can be used with attribute tagging to quickly perform useful type coercion.
  • MOJ-154 This is all of the work to get objects and arrays quoted properly for the format we'll need them in for updates. Next task is to control saves so that they omit objects/arrays on insert and then go back to update them after the fact
  • MOJ-156 This is a working methodology for automatically generating IDs. EasyIdentified can be safely included in the ApplicationRecord as it will do nothing under most circumstances. Same for InsertAttributeStripper Still need to do a little bit of cleanup, but wanted to get the code committed and pushed in it's current state.
  • MOJ-156 Removing the AutoIdentified concern. No longer necessary or useful after reworking the EasyIdentified concern.
  • MOJ-156 updates from comments Updated striped to stripped Added snowflake integer to the typemap and went ahead and updated all of the new types that were added to be specific to the odbc adapter now that I've discovered that's a thing.
  • MOJ-178 Updates to issues identified when working on MOJ-178 EasyIdentified now uses the connection of it's own class instead of using the connection of ApplicationRecord. - The connection of ApplicationRecord in springbuk is the postgres connection InsertAttributeStripped was bugged. Inserts worked fine, updates did not. Added necessary bits to fix that and realized that save and save! had grown to a considerable size with the only difference between them being the method that they called. Refactored them to call a common function and pass a method into that function so it calls the correct parent method.
  • MOJ-178 More fixes discovered while working on MOJ-178 RecordNotUnique, QueryTimeoutError, and ConnectionFailedError all inherit from StatementInvalid which no longer accepts an exception as it's second argument, but does take the SQL and binds. Use self.class for the InsertAttributeStripper transaction. ActiveRecord::Base may reference a different database - Currently in springbuk, ActiveRecord::Base goes to the postgres database. self.class will go to the database of whatever parent class (correctly going to snowflake where it's used)
  • MOJ-203 Discovered that snowflake returns slightly different types enumerations when you enable utf8.
  • changed Integer to bigInteger
  • Fix associated record saving
  • Moj 450 update with prefetch primary key ([4.2.x] Backport null adapter localytics/odbc_adapter#19)
  • Do not use bool alias (arel added in rails active record.As per ref https://github.com/rails… localytics/odbc_adapter#21)
  • MOJ-366 Two updates for added functionality (QueryTimeoutError only takes one argument. localytics/odbc_adapter#20)
  • [COR-3018] Support Rails 7 (arel gem added in active_record in rails localytics/odbc_adapter#22)
  • Moj 574 speedup hr uploads processing ([v4.2.x] Fix connection string parsing localytics/odbc_adapter#23)
  • [MOJ-578] Use new springbuk odbc-ruby gem. (Be more flexible with the arity of ActiveRecord::StatementErrors' constructor localytics/odbc_adapter#24)
  • [MOJ-586] Update odbc-ruby gem. (upgrade to AR 5.2.1 and arel 9.0.0 localytics/odbc_adapter#26)
  • Moj 519 plan years controller (Adding utf8-enabled odbc adapter localytics/odbc_adapter#27)
  • APP-3029 Fixes prune duplicates. All records with a nil primary key get their primary key replaced by a new object, which will never compare true to anything else. (Tests are passing in Docker container localytics/odbc_adapter#28)

bwarminski and others added 30 commits January 15, 2019 16:23
Tests are passing in Docker container
Reenable Rubocop, add utf8 encoding config parameter
…error to ConnectionFailedError so the caller knows to retry
Handle connection failure by trying to reconnect and translating the error to ConnectionFailedError so the caller knows to retry
Changes to support multiple databases in the Snowflake warehouse. The INFORMATION_SCHEMA tables contain information for all databases in the warehouse.
…mpatibility)

Source
======

https://springbuk-glass.atlassian.net/browse/APP-2051

Problem
=======

The linked story is the first implementation of snowflake in the main Springbuk app. Springbuk is on 6.1, and ActiveRecord 6.1 is passing ** to allow kwargs through.

In our use case, there's nothing actually being passed here, but we should just keep passing them along in the call chain.

Solution
========

Adds `**kwargs` to initialize signature and pass along to super.

Testing/QA Notes
================

Tested locally in springbuk (6.1.4) and edison (6.0.3).

Post Merge Notes
================

n/a
…ze_for_6_1_compatibility

[APP-2051] Update Column#initialize to pass on **kwargs (rails 6.1 compatibility)
…ions

[MOJ-92] Corrections for handling names
[MOJ-93] Correct SchemaStatements.columns
[MOJ-95] Corrections to the columns function
ACerka-Springbuk and others added 24 commits April 28, 2022 13:27
EasyIdentified can be safely included in the ApplicationRecord as it will do nothing under most circumstances.
Same for InsertAttributeStripper
Still need to do a little bit of cleanup, but wanted to get the code committed and pushed in it's current state.
…seful after reworking the EasyIdentified concern.
Updated striped to stripped
Added snowflake integer to the typemap and went ahead and updated all of the new types that were added to be specific to the odbc adapter now that I've discovered that's a thing.
…StructuredDataAndAutomaticIds

Moj 154 & 156 inserting structured data and automatic ids
EasyIdentified now uses the connection of it's own class instead of using the connection of ApplicationRecord.
- The connection of ApplicationRecord in springbuk is the postgres connection
InsertAttributeStripped was bugged. Inserts worked fine, updates did not.
Added necessary bits to fix that and realized that save and save! had grown to a considerable size with the only difference between them being the method that they called.
Refactored them to call a common function and pass a method into that function so it calls the correct parent method.
RecordNotUnique, QueryTimeoutError, and ConnectionFailedError all inherit from StatementInvalid which no longer accepts an exception as it's second argument, but does take the SQL and binds.
Use self.class for the InsertAttributeStripper transaction. ActiveRecord::Base may reference a different database
- Currently in springbuk, ActiveRecord::Base goes to the postgres database. self.class will go to the database of whatever parent class (correctly going to snowflake where it's used)
…stionsWorking

Moj 178 get refresh questions working
…rtBigInts

MOJ-224 - changed Integer to bigInteger
…d-records-saving

MOJ-225 fix associated records saving
* MOJ-343 Active Record does support, even if it's not well documented, a method of automatically getting IDs from the database. Implemented this native method to deal with an issue with ActiveStorage.

* MOJ-450 Removed the EasyIdentified concern & corrected a minor bug
* MOJ-366 Two updates for added functionality
- Varient & object fields now support store_accessor attribute on models
- Insert attribute stripper now validates the record before stripping the attributes, but then disables validation when doing the initial save so that it doesn't fail validation when required attributes are stripped off.

* MOJ-366 Switched InsertAttributeStripper back to use calling the base function, so that it will always correctly return false or raise the error depending on if save or save! is called.

* MOJ-366 Need to use options, not temp_options here. temp_options doesn't even exist yet.
* Support add_column in migrations

* Remove testing code
* MOJ-574 Updates to the ODBC adapter for a new merge_all functionality. Similar functionality to active record's insert/upsert all, but uses the SQL merge syntax instead of insert with update capabilities.

* MOJ-574 Fixes to the rails 7 changes and added the ability to prune duplicate records in merge_all

* MOJ-574 Removed extra debug statement

* MOJ-574 Optimization to reduce memory and/or cpu usage. Large merges are killing sidekiq.

* MOJ-574  Cleaning up the delete code, but ultimately leaving it intentionally disabled and inaccessible. It'll get worked on/tested if/when it's needed.

* MOJ-574 I forgot to remove the delete_key code from the persistence portion of the adapter...

* MOJ-574 And forgot to correct delete_keys in updatable and insertable columns methods
* Use new springbuk odbc-ruby gem.

* Increment version.
Update odbc-ruby gem.
Update version number.
* MOJ-519 Rewrote the column retrieval to use the information schema instead of odbc calls so we can get default values

* MOJ-519 Removed unnecessary check for empty string default value

* MOJ-519 Removed the idea of the table store. May be worth revisiting later on, but with multiple instances of the connection I'm not confident that the small gains are worthwhile.

* MOJ-519 Committing code with debug still in place

* MOJ-519 Removed the debug code and made the private methods private

* MOJ-519 Remove case changes from the columns query so it can be used with quoted table names
…et their primary key replaced by a new object, which will never compare true to anything else. (localytics#28)
@divoxx divoxx marked this pull request as ready for review August 14, 2023 18:45
@divoxx
Copy link
Member Author

divoxx commented Aug 21, 2023

There is no one who have the context to review this PR. However, this mainly pull bug fixes and improvements from Springbuk's fork.

Copy link
Member

@austenmadden austenmadden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was interesting reading through this code and the upstream some of the changes were derived from; Knowing how much relatively obscure community work this custom adapter relies on does make me a little nervous about the maintenance burden this could become but also the potential upsides are hard to ignore IMO.

Thanks for doing this work, intrigued to see where this goes.

@divoxx
Copy link
Member Author

divoxx commented Aug 22, 2023

@austenmadden thank you so much for taking the time and reading it through.

It's very unfortunate that the upstream driver looks abandon, but at least some people are putting some effort towards they own forks. Wand was already using our own fork, so figured I'd just pull the most recent fixes and improvements into it, and see how well it would work.

It is something to keep in mind, though. We might need to maintain this in the future, and there is no guarantee that someone else will do the work. However, AR adapters don't change that often, so it shouldn't really require a lot of upkeep.

@divoxx divoxx merged commit 0ac147f into master Aug 22, 2023
@divoxx divoxx deleted the rk-pull-fixes-from-springbuk-fork branch August 22, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.